Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Jetpack Focus] Revert Jetpack Notifications Disabling Feature Flag and Release Notes #19735

Merged
merged 4 commits into from
Dec 6, 2022
Merged

[Jetpack Focus] Revert Jetpack Notifications Disabling Feature Flag and Release Notes #19735

merged 4 commits into from
Dec 6, 2022

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Dec 5, 2022 •
edited by twstokes
Loading

Description

This PR that disables functionality of "Disable WordPress Notifications" project

Reverting #19619

Internal ref: D94385-code

Testing instructions

Prerequisites

Confirm that prevent_duplicate_notifs_remote_field":false value exists in https://public-api.wordpress.com/wpcom/v2/mobile/feature-flags

Case 1: Notifications not disabled on WordPress app

  1. Install WordPress app and log in
  2. Go to Notification tab on WordPress app and accept iOS notification permission
  3. Install Jetpack app and log in
  4. Go to Notification tab on Jetpackk app and accept iOS notification permission
  5. Jetpack app should not redirect to WordPress app
  6. Trigger push notification (write a comment on a blog)
  7. Both Jetpack and WordPress apps should receive comments

Case 2. No "Allow Notifications" switch should be visible on WordPress

  1. Install WordPress app and log in
  2. Go to Notification tab on WordPress app and accept iOS notification permission
  3. Staying on Notifications tab, click Settings icon on a top right corner
  4. Confirm that there's no "Allow Notifications" setting at the top of the settings view

Regression Notes

  1. Potential unintended areas of impact

The feature is isolated, there shouldn't be side-effects if we confirm that Jetpack app doesn't turn off WordPress notifications

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

None

  1. What automated tests I added (or what prevented me from doing so)

Existing JetpackNotificationMigrationServiceTests continues running and succeeding, that test both enabled and disabled cases

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@staskus staskus added this to the 21.3 ❄️ milestone Dec 5, 2022
@staskus staskus requested review from guarani and twstokes December 5, 2022 19:12
@twstokes
Copy link
Contributor

twstokes commented Dec 5, 2022

Hey @staskus! 👋

Could we add testing steps (and or screenshots) for the items under the "Testing" section so testers know how to get to those areas? Thanks! 🙇

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 5, 2022 •
edited
Loading

You can test the changes in Jetpack from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19735-75c637f on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 5, 2022 •
edited
Loading

You can test the changes in WordPress from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr19735-75c637f on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@twstokes twstokes mentioned this pull request Dec 5, 2022
Merged
4 tasks
@salimbraksa
Copy link
Contributor

Hey @staskus, Jetpack app is still redirecting to WP when I allowed Notifications. Not sure if I'm doing something wrong:

 Recording
CleanShot.2022-12-05.at.22.23.00.mp4

And all content migration feature flags were disabled:

@twstokes twstokes requested a review from salimbraksa December 5, 2022 23:50
twstokes
twstokes reviewed Dec 5, 2022
View reviewed changes
RELEASE-NOTES.txt Outdated Show resolved Hide resolved
@twstokes twstokes force-pushed the task/disable-prevent-notifications branch 2 times, most recently from e6066bc to f1b3975 Compare December 6, 2022 00:22
@twstokes twstokes force-pushed the task/disable-prevent-notifications branch from f1b3975 to a325d3d Compare December 6, 2022 00:34
@twstokes
Copy link
Contributor

twstokes commented Dec 6, 2022

Hey @staskus, Jetpack app is still redirecting to WP when I allowed Notifications. Not sure if I'm doing something wrong:

I can confirm the same behavior running from the latest on this branch.

jp-to-wp.mp4

@staskus
Copy link
Contributor Author

staskus commented Dec 6, 2022

@twstokes @salimbraksa it happened because the remote flag is still enabled and it overrides a local flag. Sorry for not mentioning it in the PR. There's a separate diff that disables it which is made but not yet deployed.

@staskus
Copy link
Contributor Author

staskus commented Dec 6, 2022

Hey @staskus! 👋

Could we add testing steps (and or screenshots) for the items under the "Testing" section so testers know how to get to those areas? Thanks! 🙇

@twstokes okay. This was super quick PR before EOD prioritizing quick merge over the quality of description.

@twstokes
Copy link
Contributor

twstokes commented Dec 6, 2022

@twstokes @salimbraksa it happened because the remote flag is still enabled and it overrides a local flag. Sorry for not mentioning it in the PR. There's a separate diff that disables it which is made but not yet deployed.

Ah, that makes sense! Thanks for the clarification.

@twstokes okay. This was super quick PR before EOD prioritizing quick merge over the quality of description.

Totally understood. Thanks for the updates. 👍

@twstokes
Copy link
Contributor

twstokes commented Dec 6, 2022 •
edited
Loading

I've added the remote feature flag PR to the description of this one. I assume it will need to be merged before we can proceed with testing and merging this one.

Update: It was decided to override the remote functionality for this feature in 21.3, so the API change has been cancelled.

Ref D94385-code

@twstokes twstokes self-requested a review December 6, 2022 13:51
twstokes
twstokes approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @staskus. 🚀

  • Jetpack did not bring WordPress to the foreground when allowing notifications
  • I received push notifications on both apps
  • I did not see the "Allow Notifications" switch in WordPress

salimbraksa
salimbraksa approved these changes Dec 6, 2022
View reviewed changes
Copy link
Contributor

@salimbraksa salimbraksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm this is working as expected. LGTM! 🚀

@twstokes twstokes merged commit fa14718 into release/21.3 Dec 6, 2022
@twstokes twstokes deleted the task/disable-prevent-notifications branch December 6, 2022 14:29
@twstokes twstokes mentioned this pull request Dec 6, 2022
Merged
@mokagio mokagio mentioned this pull request Dec 7, 2022
Merged
3 tasks
@staskus staskus mentioned this pull request Dec 9, 2022
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@salimbraksa salimbraksa salimbraksa approved these changes

@twstokes twstokes twstokes approved these changes

@guarani guarani Awaiting requested review from guarani

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
21.3 ❄️
Development

Successfully merging this pull request may close these issues.
4 participants
@staskus @twstokes @wpmobilebot @salimbraksa